-
Notifications
You must be signed in to change notification settings - Fork 147
Add Block Device Support #191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
ba574e8
to
73f4574
Compare
Add ls, mkdir, put and get commands to access block devices Currently only supports littlefs
…lity to specify directory
Default to writeable and formattable, but override based on bdev flags, or partition NSBOOT permissions
Adds ls and cp commands for FatFS
…ranslation Layer (prevents doing lots of erases) Change Fat sector size back to 512, and consequently add in flash translation error - wasn't needed before with 4096 sector size Also enable LFN support
73f4574
to
c49b5d1
Compare
Need to put BLOCK_DEVICE_PARTITION_ID into the SDK somewhere
README.md
Outdated
Specify filesystem to use | ||
<fs> | ||
littlefs|fatfs | ||
-f, --format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, we now seem to have BOTH -f, --format
and -f, --force
flags?! Either this worn't work properly, or if it does work it's pretty confusing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot - I’ll make this filesystem one not have a short argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(perhaps the arg-parsing library could/should raise an error if given the same argument multiple times?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(perhaps the arg-parsing library could/should raise an error if given the same argument multiple times?)
I will leave that for a future PR
README.md
Outdated
|
||
### cp | ||
|
||
Copy file to/from the block device - use :filename to indicate files on the device (eg `cp main.py :main.py` to upload to the device) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Does this
bdev cp
command check that one (and only one) of eithersrc
ordest
starts with a colon? - Does it only work for files and not directories?
- If the answer to the previous question is "only files", would it be a lot of additional work to be able to copy directories? (which would presumably need to operate recursively)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- No - you can copy a file to somewhere else on the device, or even use this to copy local files if you really wanted
- Yes, it only works for files
- Could probably add that - just needs a recursive walk like
ls
has
main.cpp
Outdated
|
||
group get_cli() override { | ||
return ( | ||
optional_untyped_file_selection_x("dirname", 0) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the optional_
here indicate that this is an optional argument? If so, it should probably appear in the README.md
as [dirname]
rather than <dirname>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it’s optional - I’m not sure why the CLI stuff isn’t putting square brackets around it
A thought I just had - to make this slightly easier to use, could you have |
I think that might be best left to the user to do, to avoid polluting their shell (eg you might want |
Add extra checks to prevent too big files Fix dirname description Remove short format option Improve erase check (rather than just checking for FF)
--format | ||
Format the drive if necessary (may result in data loss) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably using the --format
option in conjunction with picotool bdev ls
will always return an empty directory listing? 😂 But this makes me wonder why --format
is an option that can be used in combination with any other picotool bdev
command, rather than just being a standalone picotool bdev format
or picotool format_bdev
command? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it as a flag because of where the formatting occurs in the code - it will only format the drive if no filesystem (of the type specified by --filesystem
if that is passed) is detected, so you could always call your first command with --format
if you wanted to make sure you always do the command. Eg this works for both formatting new devices and updating the file on old devices:
picotool bdev cp main.py :/ --format --filesystem littlefs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I see now that I'd misread "Format the drive if necessary" as just "Format the drive" 🤦
So I guess that means if you had a blockdevice that was already fatfs-formatted, and you wanted to explicitly reformat it, you'd need to do something like:
picotool bdev ls --format --filesystem littlefs
picotool bdev ls --format --filesystem fatfs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, so it probably is worth adding a separate picotool bdev format
command to unconditionally format the block device
@@ -94,6 +97,10 @@ static __forceinline int __builtin_ctz(unsigned x) { | |||
#ifndef BOOTROM_FAMILY_ID_MAX | |||
#define BOOTROM_FAMILY_ID_MAX RP2350_ARM_NS_FAMILY_ID | |||
#endif | |||
#ifndef BLOCK_DEVICE_PARTITION_ID | |||
// The default 0x626C6F636B646576 value is the ASCII encoding of "blockdev" | |||
#define BLOCK_DEVICE_PARTITION_ID 0x626C6F636B646576 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this ID also appear somewhere in a pico-sdk header, in case 3rd-party utilities also want to use it? Or do we expect any 3rd-party utilities to just copy'n'paste this into their own source code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if it's decided that this is wanted as a way to identify block device partitions, then I'll open an SDK PR to add this to the SDK
Adds support for using
ls
,mkdir
,cp
(including between host and device),rm
, andcat
commands, with LittleFS (used by MicroPython) or FatFS filesystem support.Works with block devices specified in the binary info, and with partitions. It autodetects the file system if
--filesystem
is not specified, and also supports formatting the block device if required with the-f
argument.This fixes #58